Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

test,doc: address http2 connection abort/network error handling #21861

Closed
wants to merge 4 commits into from
Closed

test,doc: address http2 connection abort/network error handling #21861

wants to merge 4 commits into from

Conversation

addaleax
Copy link
Member

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@addaleax addaleax added doc Issues and PRs related to the documentations. test Issues and PRs related to the tests. http2 Issues or PRs related to the http2 subsystem. labels Jul 17, 2018
@nodejs-github-bot
Copy link
Collaborator

@addaleax build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/298/pipeline

Trott
Trott reviewed Jul 17, 2018
View reviewed changes
doc/api/http2.md Outdated
stream.end('<h1>Hello World</h1>');
});

server.listen(80);
```

Even though HTTP/2 streams and network sockets are not in a 1:1 correspondence,
a network error will destroy each individual streams and must be handled on the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: streams -> stream?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, done!

@Trott
Copy link
Member

Trott commented Jul 18, 2018

@nodejs/http2

vsemozhetbyt
vsemozhetbyt reviewed Jul 18, 2018
View reviewed changes
doc/api/http2.md Outdated
@@ -271,12 +271,17 @@ server.on('stream', (stream, headers) => {
'content-type': 'text/html',
':status': 200
});
stream.on('error', (error) => console.log(error));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

console.log -> console.error?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe just:

stream.on('error', console.error);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Switched to the first suggestion – I’d prefer to give a user-defined function here, since that is a bit closer to what one would actually do in real-world code

mcollina
mcollina approved these changes Jul 18, 2018
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 18, 2018
@addaleax
Copy link
Member Author

addaleax commented Jul 18, 2018 •
edited
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/15930/ (edit: ✔️)

@addaleax addaleax mentioned this pull request Jul 18, 2018
Closed
2 tasks
lpinca
lpinca approved these changes Jul 19, 2018
View reviewed changes
doc/api/http2.md
@@ -271,12 +271,17 @@ server.on('stream', (stream, headers) => {
'content-type': 'text/html',
':status': 200
});
stream.on('error', (error) => console.error(error));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: how about passing console.error directly? It's already bound. If this is the prevalent usage in the docs, ignore this comment.

Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Jul 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems this is a deliberate decision.

@addaleax
Copy link
Member Author

Landed in daa15b5, 0e4cbde

@addaleax addaleax closed this Jul 23, 2018
@addaleax addaleax deleted the http2-aborted-connection branch July 23, 2018 15:05
addaleax added a commit that referenced this pull request Jul 23, 2018
@addaleax
test: fix http2 connection abort test
daa15b5 
Refs: #21836
Refs: #21561

PR-URL: #21861
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
addaleax added a commit that referenced this pull request Jul 23, 2018
@addaleax
doc: document http2 network error behaviour
0e4cbde 
Fixes: #21836

PR-URL: #21861
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
targos pushed a commit that referenced this pull request Jul 24, 2018
@addaleax @targos
test: fix http2 connection abort test
eea199b 
Refs: #21836
Refs: #21561

PR-URL: #21861
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
targos pushed a commit that referenced this pull request Jul 24, 2018
@addaleax @targos
doc: document http2 network error behaviour
c3d9000 
Fixes: #21836

PR-URL: #21861
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
@targos targos mentioned this pull request Jul 31, 2018
Merged
This was referenced Aug 1, 2018
Closed
Closed
This was referenced Aug 1, 2018
Closed
Closed
Closed
Closed
kjin pushed a commit to kjin/node that referenced this pull request Aug 23, 2018
@addaleax @kjin
doc: document http2 network error behaviour
8b03609 
Fixes: nodejs#21836

PR-URL: nodejs#21861
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
@kjin kjin mentioned this pull request Sep 19, 2018
Closed
4 tasks
@addaleax addaleax mentioned this pull request Oct 20, 2018
Closed
2 tasks
This was referenced Jun 18, 2021
Open
Open
Open
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vsemozhetbyt vsemozhetbyt vsemozhetbyt left review comments

@Trott Trott Trott left review comments

@trivikr trivikr trivikr approved these changes

@JungMinu JungMinu JungMinu approved these changes

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations. http2 Issues or PRs related to the http2 subsystem. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@addaleax @nodejs-github-bot @Trott @mcollina @jasnell @lpinca @JungMinu @vsemozhetbyt @trivikr